Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Manifest files storage #166

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Manifest files storage #166

wants to merge 6 commits into from

Conversation

jrief
Copy link
Owner

@jrief jrief commented Jan 19, 2023

This branch is based on version 1.2.1 which is able to handle the manifest static files storage properly.

  • A unit test to check if the former works has been added.
  • All unit tests now use pathlib.Path instead of os.path....

@Natureshadow
Copy link
Collaborator

This MR adds unnecessary complexity and special-casing for one storage class. We should not do that.

As in the previous I-don't-know-how-many iterations of this topic: The ManifestStaticFilesStorage works perfectly without any changes.

I am pushing an MWE right now, and waiting for @jrief to disprove that it works.

@Natureshadow
Copy link
Collaborator

So, here you go:

https://github.com/Natureshadow/sass_mwe

❯ git clone https://github.com/Natureshadow/sass_mwe
Cloning into 'sass_mwe'...
remote: Enumerating objects: 16, done.
remote: Counting objects: 100% (16/16), done.
remote: Compressing objects: 100% (11/11), done.
remote: Total 16 (delta 1), reused 16 (delta 1), pack-reused 0
Receiving objects: 100% (16/16), 7.91 KiB | 7.91 MiB/s, done.
Resolving deltas: 100% (1/1), done.
❯ cd sass_mwe
❯ poetry install
Creating virtualenv sass-mwe in /home/nik/sass_mwe/.venv
Installing dependencies from lock file

Package operations: 9 installs, 0 updates, 0 removals

  • Installing asgiref (3.6.0)
  • Installing sqlparse (0.4.3)
  • Installing django (4.1.5)
  • Installing django-appconf (1.0.5)
  • Installing rcssmin (1.1.1)
  • Installing rjsmin (1.2.1)
  • Installing django-compressor (4.3)
  • Installing django-sass-processor (1.2.2)
  • Installing libsass (0.22.0)

Installing the current project: sass-mwe (0.1.0)
❯ poetry run ./manage.py compilescss
....................................................................................................................................................................................................
Successfully compiled 1 referred SASS/SCSS files.
❯ poetry run ./manage.py collectstatic

132 static files copied to '/home/nik/sass_mwe/static', 132 post-processed.
❯ poetry run ./manage.py runserver &
[1] 50895
    ~/sass_mwe    master !1 ?2  Performing system checks...                                                           ✔    ICE858 🚄 

System check identified no issues (0 silenced).

You have 18 unapplied migration(s). Your project may not work properly until you apply the migrations for app(s): admin, auth, contenttypes, sessions.
Run 'python manage.py migrate' to apply them.
January 19, 2023 - 13:38:34
Django version 4.1.5, using settings 'sass_mwe.settings'
Starting development server at http://127.0.0.1:8000/
Quit the server with CONTROL-C.

❯ curl http://127.0.0.1:8000/ 2>/dev/null | grep link
        <link href="/static/style.b4b34d15db26.css" rel="stylesheet" type="text/css" />

In order for me to understand why this change is needed, please prove that this simple MWE yields a different result for you.

@Natureshadow
Copy link
Collaborator

As per the comment in Natureshadow/sass_mwe#1, the issue occurs when recompiling CSS during runtime, instead of properly building for deployment. IMHO, rebuilding application code (that includes stylesheets!) during runtime in production is an anti-patern in itself. Ironically, the reason I am using an S3 storage is actually because I need to rebuild stylesheets during runtime in production :D (due to limitations of Materialize).

Yet, this still doesn't mean we need a new storage class as a special case for the manifest storage. The processor should simply call .post_process() on the storage after building if the storage has that method, and that behaviour should probably we controllable by a setting.

@jrief
Copy link
Owner Author

jrief commented Jan 19, 2023

As per the comment in Natureshadow/sass_mwe#1, the issue occurs when recompiling CSS during runtime, instead of properly building for deployment.

I compile all my SCSS files and collect all my CSS files during the build process of the Docker container used for deployment.

The processor should simply call .post_process() on the storage after building

that's too late. The error occurs before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jrief @Natureshadow